-
-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(plugins/worker): support SharedWorker (resolve #2093) #2505
Conversation
c9a21b2
to
00f4f72
Compare
Don't know why appveyor failed. Seems like an unrelated failure. |
if ( | ||
isBuild && | ||
(parseWorkerRequest(id)?.worker ?? | ||
parseWorkerRequest(id)?.sharedworker) != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick (non-blocking): I think the second optional chaining isn't needed, cause it's already checked in the first check
parseWorkerRequest(id)?.sharedworker) != null | |
parseWorkerRequest(id).sharedworker) != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first optional chaining checks worker
property of qs parsed search query. We still need to check for sharedworker
, right? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(parseWorkerRequest(id)?.worker ?? parseWorkerRequest(id).sharedworker) != null
// Is the same as
(parseWorkerRequest(id)?.worker ?? parseWorkerRequest(id)?.sharedworker) != null
// I think it becomes a bit clearer this way
const temp = parseWorkerRequest(id)
(temp?.worker ?? temp.sharedworker) != null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you are right, I just made a mistake. I like your recommended style, but TS complains with single ?.
. TS won't know if because temp
is nullish or temp.worker
is nullish so it requires write ?.
again.
I think we had to use double ?.
for type checking. Or checks parsedQuery
in advance, but maybe a little verbose.
if (
isBuild &&
(parsedQuery && (parsedQuery.worker ?? parsedQuery.sharedworker)) !=
null
) {
return ''
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah you are right (I was wrong I think)
Cause if parsedQuery
would be undefined
, then it falls into the second case and would be also undefined
there
But I think your last example checking parsedQuery
first could also be a solution
we loose short circuit here when calling the function before isBuild
was checked
so maybe the best solution of both worlds would be to nest two if
s
if (isBuild) {
const parsedQuery = parseWorkerRequest(id)
if (parsedQuery && (parsedQuery.worker || parsedQuery.sharedWorker)) {
// maybe: if (parsedQuery && (!!parsedQuery.worker || !!parsedQuery.sharedWorker)) {
return ''
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a little change.
parsedQuery.worker ?? parsedQuery.sharedworker
Should use ??
because the value will be ''
if the property exists.
!= null
Use != null
to distinguish between ''
and undefined
5630c3e
to
5e2bb4d
Compare
@fi3ework Could you please update your PR by merging the latest |
@Shinigami92 Rebased. No conflict and test passed. |
@patak-js Please take a look at this PR, maybe give some advice on the new test case. 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for splitting the options. We should document this feature here: https://vitejs.dev/guide/assets.html#importing-asset-as-string
@patak-js Appended |
Thanks for the work here @fi3ework. Evan added the contribution welcome to this one in the issue, so we can merge it once we get enough approvals 👍🏼 |
It seems like jest v27 breaks the test. Looking into it |
Resolve #2093
Uses test.concurrent to test shared worker operation. It's still an experimental API. May there's a better way to do this test (or maybe add test for the compiled worker file instead of E2E test). 🤔
Update 1
use test.concurrent.each, this API is stable.